cli: add SNP ID block annotations to Pods based on CPU requirements#2214
cli: add SNP ID block annotations to Pods based on CPU requirements#2214daniel-weisse wants to merge 8 commits intomainfrom
Conversation
daniel-weisse
force-pushed
the
dw/cli-id-block-generation
branch
7 times, most recently
from
2287b59 to
6de728d
Compare
daniel-weisse
force-pushed
the
dw/cli-id-block-generation
branch
3 times, most recently
from
3acb557 to
6fd606e
Compare
daniel-weisse
force-pushed
the
dw/cli-id-block-generation
branch
2 times, most recently
from
0304e90 to
f63b942
Compare
daniel-weisse
force-pushed
the
dw/cli-id-block-generation
branch
2 times, most recently
from
59f9a72 to
cf6cec9
Compare
msanft
left a comment
msanft
left a comment
There was a problem hiding this comment.
I think this will work, but I'm a little unsure about the current interface we expose to the user.
| podLevelCPU := getCPUCount(spec.Resources) | ||
|
|
||
| // Convert milliCPUs to number of CPUs (rounding up), and add 1 for hypervisor overhead | ||
| totalMilliCPUs := max(regularContainersCPU, initContainersCPU, podLevelCPU) |
There was a problem hiding this comment.
I wonder if this matches the user's expectations, or what's done by non-Kata Kubernetes here.
There was a problem hiding this comment.
What do you think may be unexpected about this formula? I pointed @daniel-weisse to #2272 for where it comes from.
There was a problem hiding this comment.
The thing I was wary about is the round-up. With cgroups and CPU slices, this isn't something to worry about. But when a user shifts some YAML that worked in his non-Contrast deployment to Contrast, we may try to use more CPUs than physically available due to this. I don't think this is something that would be a realistic scenario, though. LMK
There was a problem hiding this comment.
Understood, thanks. We'll need to document this in https://docs.edgeless.systems/contrast/howto/workload-deployment/deployment-file-preparation#pod-resources before we consider this feature done, yes. I don't see what we could do to not round up, though, since fractional CPUs don't make sense for VMs.
There was a problem hiding this comment.
Scheduler considerations might become interesting, though: I don't think there's a way to tell k8s via runtimeClass to round up the limits.
There was a problem hiding this comment.
Do you have a concrete idea on how to proceed with this? I don't see what we could do either.
There was a problem hiding this comment.
Just document it, recommeding only integral CPU counts. If rounding does not change the number, there are no problems with unexpected counts or scheduling. But if the user decides to go against that recommendation, this code still does the right thing.
|
@burgerdev, @charludo; Addressed my own feedback, PTAL. |
charludo
left a comment
charludo
left a comment
There was a problem hiding this comment.
fixup changes LGTM; have not looked into the still-open conversation.
charludo
force-pushed
the
dw/cli-id-block-generation
branch
from
0540eab to
19720c8
Compare
Signed-off-by: Daniel Weiße <dw@edgeless.systems>
Signed-off-by: Daniel Weiße <dw@edgeless.systems>
Signed-off-by: Daniel Weiße <dw@edgeless.systems>
Signed-off-by: Daniel Weiße <dw@edgeless.systems>
Not specifying `nr_cpus` on the command line costs us marginal amounts of memory while saving complexity in the TDX RTMR pre-calculation. By dropping this from the command line, we make the kernel fall back to the `CONFIG_NR_CPUS=240` kconfig variable.
charludo
force-pushed
the
dw/cli-id-block-generation
branch
from
19720c8 to
61a5bd1
Compare
charludo
force-pushed
the
dw/cli-id-block-generation
branch
from
61a5bd1 to
863a238
Compare
charludo
force-pushed
the
dw/cli-id-block-generation
branch
from
863a238 to
9c5afa4
Compare
5 participants
contrast generatewith the ID blocks required for the requested CPU amount